RFC: An effort to standardize OP-TEE rust based TAs development environment #114
Conversation
|
@DemesneGH @Sword-Destiny @jbech-linaro @jenswi-linaro @jforissier @etienne-lms @daniel-thompson @Ablu Fyi.. Feedback/comments are very much welcome, thanks. |
|
@b49020 Thanks so much for your contributions! Since the BTW please check out the failed CI tasks, thanks! |
|
Thanks @DemesneGH for your comments.
Sure I am open to merging this in separate branch for now but I would like to see
My bad, I forgot to update linker configuration for host applications. The build didn't show any issue for me since the host applications weren't rebuilt. BTW, the issue should be fixed now, please re-run CI now. |
|
Rather than create a branch is it possible to introduce a no_std feature to optee-utee crate (similar to crates like serde which support no_std but it is optional). That would allow progressive adoption of no_std without having to fork. Cargo can handle some quite complex feature combinations, including marking up examples that require std-only crates so you don't need to enable them crate wide. |
Good point. I agree with you. |
|
@b49020 Seems there are some license issue on CI: |
That sounds fine but for |
|
|
The current CI failure is due to following missing patch to OP-TEE build repo as I have stated above: |
There was a problem hiding this comment.
Having almost zero context on the previous Optee Rust SDK, this looks like a very neat cleanup!
Just some random comments.
It looks like there is interest to optimize for size. Since you are using nightly anyway, you may want to try:
build-std = ["core", "alloc"]
details: https://doc.rust-lang.org/cargo/reference/unstable.html
That will rebuild core and alloc as part of your build, which allows them to to be rebuilt with -Os too. That might further improve the binary sizes.
Disclaimer: I have not played with this for no_std binaries so far.
| println!("cargo:rustc-link-arg=-pie"); | ||
| println!("cargo:rustc-link-arg=-Os"); | ||
| println!("cargo:rustc-link-arg=--sort-section=alignment"); | ||
| println!("cargo:rustc-link-arg=--dynamic-list=dyn_list"); |
There was a problem hiding this comment.
One could probably move the boilerplate code into a crate that is only used as dev-dependencies to avoid the duplication.
There was a problem hiding this comment.
Sure, I will look for ways to move this into optee-utee crate.
|
Thanks @Ablu for your review.
Binary size reduction is not such a hard requirement here. However, from security point of view its better to reduce code footprint which this |
@b49020 I see. I think after upstreaming the |
|
@DemesneGH How about we rather stick the This will atleast allow the adoption of |
The patch has been posted here: OP-TEE/build#714 |
|
@b49020 do you need a new release on https://crates.io/crates/optee-utee? |
Default feature remains std support, no_std can be enabled optionally. Signed-off-by: Sumit Garg <[email protected]>
Default feature remains std support, no_std can be enabled optionally. Signed-off-by: Sumit Garg <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
Drop Box corresponding to the handle which has been freed to avoid following warning: warning: unused return value of `Box::<T>::from_raw` that must be used Signed-off-by: Sumit Garg <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
While at it drop redundant Cargo.lock Signed-off-by: Sumit Garg <[email protected]>
While at it drop redundant Cargo.lock Signed-off-by: Sumit Garg <[email protected]>
While at it drop redundant Cargo.lock Signed-off-by: Sumit Garg <[email protected]>
While at it drop redundant Cargo.lock Signed-off-by: Sumit Garg <[email protected]>
While at it drop redundant Cargo.lock Signed-off-by: Sumit Garg <[email protected]>
While at it drop redundant Cargo.lock Signed-off-by: Sumit Garg <[email protected]>
While at it drop redundant Cargo.lock Signed-off-by: Sumit Garg <[email protected]>
While at it drop redundant Cargo.lock Signed-off-by: Sumit Garg <[email protected]>
In no_std environment we have to explicitly link against libutils provided by OP-TEE. Signed-off-by: Sumit Garg <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
Along with that drop global .cargo/config and move linker specific configuration to TA build.rs file and Makefile. This allows TA directory to be self sufficient to build up corresponding TA binary. Signed-off-by: Sumit Garg <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
Having copies of TA linker scripts for every TA written in rust isn't scalable and prone to version skews with the linker script provided by TA devkit. And we would like to build and run TAs with different OP-TEE versions. So lets reuse linker script provided by TA devkit and therby drop copies from every TA direcetory. Signed-off-by: Sumit Garg <[email protected]>
This will allow us to upgrade toolchain for non_std TAs while keeping std TAs tied to a fixed nightly release until corresponding rust toolchain target lands upstream. Signed-off-by: Sumit Garg <[email protected]>
Upgrade rust toolchain to use nightly-2023-12-18 release. Signed-off-by: Sumit Garg <[email protected]>
Only no_std mode is supported with latest nightly, so build it for the time being. Signed-off-by: Sumit Garg <[email protected]>
|
@daniel-thompson @DemesneGH So I gave I hope this is something we can agree upon, feedback/comments are very much welcome. |
Yeah once we have sufficient functionality implemented to enable standalone rust TAs development. I suppose we still lack inter TA communication rust library APIs, right? |
|
@DemesneGH The CI reports |
The Github actions provide at least 14G storage (reference: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources). It would be more than 14GB sometimes (actions/runner-images#2840 (comment)) since the error haven't occurred before. There are some workaround on actions/runner-images#2840 (comment) but I'd prefer to remove the dependencies of I'm trying to summarize the conclusions we reached and the steps for breaking down, please correct me if I've misunderstood: STAGE 1:
comments:
STAGE 2 (after the STAGE 1 is finished):
|
|
@DemesneGH I mostly agree with your stage 1 but for stage 2 I think once we have published This would avoid any divergence and allow the std counterpart to be regularly tested with latest If you agree then please feel free to create a separate branch (std or no-std). |
So once we have the |
Sure, after we finished the stage 1 which means the |
Fair enough, let's go with |
|
@DemesneGH Created PR for |
|
@DemesneGH With
Although I agree it is an important feature which I will target next but I still think we can have
I don't think we should bundle a script to build OP-TEE OS since it can be configured in many ways for different platforms. We should rather depend on |
Fyi.. #116 |
This might help: OP-TEE/optee_os@788069f (although minimizing the cloned data is certainly a good idea regardless). |
With OP-TEE/manifest#260, the rust build would be significantly lighter for OP-TEE CI. Given that I am also thinking that we should probably get rid of |
Perhaps just make it |
That sounds reasonable. I will do that as a next step to current rust examples build refactoring.
How about if we rather add |
OK thanks.
Yes, much better 👍 |
Existing OP-TEE rust environment required a custom rust toolchain target for OP-TEE based TAs. I suppose back in 2019 when this SDK was created, rust embedded ecosystem (especially no_std support) was in its very early stages of development. But as of today many rust crates have already added support for rust no_std or are being actively worked on to add rust no_std support for example rustls here(rustls/rustls#1399).
This effort is a followup effort to the discussion here (#113). The major motivation for this effort was to make OP-TEE rust TAs development environment to be the first class citizen. Rust
no_stdsupport seems to provide quite similar environment as we have on the C counterpart side in OP-TEE (we don't support fully fledged libc/glibc but rather our own quite simple libutils library).Upsides for this PR:
Downsides for this PR:
Testing
Their is one change needed for OP-TEE build repo in order to build this PR as follows. Once there is consensus on this PR, I will submit this change as well.
Once that's done we should be able to build OP-TEE buildroot setup with rust support:
For interactive run, just bring up Qemu with below command and run rust examples:
Or you can test all rust examples in one go:
Performance comparisons
After this PR, the TA performance becomes equivalent to the C counterparts. This is impressive improvement as compared to 35% performance gap earlier as illustrated here (#89).
See below comparison after this PR:
Size comparisons
As you can observe from the comparisons below, there is approx. 70K - 80K TA binary size reduction after this PR:
Before this PR: